utils_test: created function to update default kernel by rh-jugraham · Pull Request #4070 · avocado-framework/avocado-vt

rh-jugraham · 2025-03-04T19:26:14Z

Created function to update default kernel

rh-jugraham · 2025-03-04T19:30:41Z

@smitterl As per discussion. Review would be appreciated!

Used update_boot_option() function right above in the file as inspiration.

dzhengfy · 2025-03-07T09:10:59Z

            session.close()


+def update_default_kernel(


Could we rename to update_vm_default_kernel()?

Usually we do not add new functions to init.py any more. This file is already too big.
Could you please move this function into utils_sys.py?

I know there is update_boot_option() in init.py, too. Actually I prefer to move it out of the file, too, but we can do it later in another pr.

Maybe for reuse of existing sessions in test code we could expect session as parameter instead.

@smitterl Since rebooting will result in a new session, would this imply also returning the session (either the updated one or, if no reboot, then the passed in one) for more reuse?

Also, I believe the vm needs to be a param because of, most importantly, vm.reboot() unless there is an alternative that doesn't require the vm. So the session would need to be in addition to the vm, not instead of.

dzhengfy · 2025-03-17T13:52:17Z

@smitterl , I feel if update_boot_option() can be moved to another file, that will be better. So I do not suggest we add new function here. Not sure about your opinion. Welcome your feedback. Thanks

smitterl · 2025-03-17T15:11:32Z

@smitterl , I feel if update_boot_option() can be moved to another file, that will be better. So I do not suggest we add new function here. Not sure about your opinion. Welcome your feedback. Thanks

I proposed to add it here because it's semantically related to the boot parameter update function but I wouldn't insist on it.

If making __init__.py smaller is the motivation then I'd be in favor of moving the two related functions into utils_sys.py but importing the old one (update boot parameters) in __init__.py. IIUC, this way existing code that uses it doesn't have to be updated but the two semantically related functions woudl live together in utils_sys.py which might also aid code exploration as opposed to keeping them in the filename __init__.py, ie.

# cat __init__.py
...
from utils_sys import update_boot_option

# cat utils_sys.py
...
def update_boot_option(...

def update_default_kernel(...

rh-jugraham · 2025-03-19T22:18:03Z

@smitterl @dzhengfy Created new PR - #4087 - for moving update_boot_option that depends on this PR (for shared imports/helper functions)

dzhengfy · 2025-03-20T07:05:56Z

@smitterl @dzhengfy Created new PR - #4087 - for moving update_boot_option that depends on this PR (for shared imports/helper functions)

@rh-jugraham @smitterl I think more about this change. Moving update_boot_option() and this pr to utils_sys.py will make init.py pretty clean, but seems we also need move update_boot_option_ubuntu(), __run_cmd_and_handle_error(), check_kernel_cmdline() . Without moving them, we will have utils_test import utils_sys , and utils_sys import utils_test as well.

And if we move update_boot_option() to utils_sys.py, we have to also update tp-libvirt and tp-qemu for their reference to this function or we need still keep an encapsulating function in init.py for ease like below
in init.py

from virttest import utils_sys

def update_boot_option( ...):
   utils_sys.update_boot_option(...)

And check_kernel_cmdline() has one reference in tp-qemu (tp-libvirt does not use it) as below:
qemu/tests/boot_without_vectors.py:40: req_args = utils_test.check_kernel_cmdline(session,

Based on above analysis, it is high cost to make init.py to be pretty, at least I do not expect this happen in your PR. Maybe we use separate PR to achieve this later. So let's keep your current implementation using __init.py this time.
Please help close #4087 and thank you for submitting it anyway.

dzhengfy · 2025-03-20T07:07:53Z

@rh-jugraham you may consider to make init.py in another PR later if you would like. Thanks.

dzhengfy

LGTM

rh-jugraham · 2025-03-20T17:48:40Z

@dzhengfy Moved back to utils_test as indeed, currently the two tests are too integrated with utils_test and reliant on other functions there so it makes sense for a different PR to figure out how to best seperate out the functions.

dzhengfy

LGTM

smitterl · 2025-03-24T17:09:36Z

+        if not utils_package.package_install("grubby", session=session):
+            raise exceptions.TestError("Failed to install grubby package")
+
+        cmd = "grubby --default-kernel"


Grubby will use the kernel image path

# grubby --default-kernel /boot/vmlinuz-6.13.5-200.fc41.x86_64

That makes me wonder which format you use for kernel_version, especially considering that there's some arch-specific info in that image file name.
From line 299 I assume you expect the full kernel path as there's no manipulation.
How would a user retrieve that value before calling this function? Would it make sense for this function to construct part of the full path, e.g. expecting only 6.13.5-200.fc41?

Below is some experimentation with what --set-kernel can and can't take. So some variation is accepted. However, I wouldn't be sure how to take in "6.13.5-200.fc41" as, for aarch64, there is the 64k option, so wouldn't be sure how that would work, especially if multiple kernels match the same partial string - such as differentiating between "vmlinuz-5.14.0-570.4.1.el9_6.aarch64+64k" and "vmlinuz-5.14.0-570.4.1.el9_6.aarch64" if only "5.14.0-570.4.1.el9_6" is given.

I added "Can use the entire kernel path or just be the full kernel name." to hopefully clarify that it can be either the path or just the name, but I don't know how to use just part of the kernel name without overcomplicating the function and making incorrect assumptions.

[root@ampere-mtsnow-altramax-47 /]# grubby --default-kernel /boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64+64k # using full output from grubby [root@ampere-mtsnow-altramax-47 /]# grubby --set-default="kernel="/boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64"" The default is /boot/loader/entries/0eae46187a3c4734ad721259b7d1c2b5-5.14.0-570.4.1.el9_6.aarch64.conf with index 1 and kernel /boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64 [root@ampere-mtsnow-altramax-47 /]# grubby --default-kernel /boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64 # using full path [root@ampere-mtsnow-altramax-47 /]# grubby --set-default="/boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64+64k" The default is /boot/loader/entries/0eae46187a3c4734ad721259b7d1c2b5-5.14.0-570.4.1.el9_6.aarch64+64k.conf with index 0 and kernel /boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64+64k [root@ampere-mtsnow-altramax-47 /]# grubby --default-kernel /boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64+64k # using just full name [root@ampere-mtsnow-altramax-47 /]# grubby --set-default="vmlinuz-5.14.0-570.4.1.el9_6.aarch64" The default is /boot/loader/entries/0eae46187a3c4734ad721259b7d1c2b5-5.14.0-570.4.1.el9_6.aarch64.conf with index 1 and kernel /boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64 [root@ampere-mtsnow-altramax-47 /]# grubby --default-kernel /boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64 # using partial name (not including vmlinuz) [root@ampere-mtsnow-altramax-47 /]# grubby --set-default="5.14.0-570.4.1.el9_6.aarch64+64k" The param 5.14.0-570.4.1.el9_6.aarch64+64k is incorrect

To me it looks like /boot is not necessary but the full kernel image file name is.

In any case, I believe you don't have to worry about that, grubby can return all kernel paths and you can implement a function that helps select one and define it here right next to it and maybe also mention in the docstrings that it can be used to select the right kernel path:

def get_available_kernel_path(session, kernel_pattern): """ Looks up all available kernels on the system and returns the first one whose image file name matches a pattern ... """ s, output = utils_misc.cmd_status_output("grubby --info ALL", session=session ...) ... for line in output.split('\n'): if line.startswith("kernel=") and re.match(kernel_pattern, line): return line.split("=")[1] ...

Sounds good! I did make it so the function returns all of the kernels it found in a list in case there is some reason to find all --> perhaps might make the function more useful.

rh-jugraham · 2025-03-25T16:51:06Z

@smitterl thanks for the suggestions! I addressed them all, except for 1 I had a question about.

… all kernel paths given some pattern Signed-off-by: Julia Graham <jugraham@redhat.com>

smitterl

LGTM thank you very much for this!

rh-jugraham force-pushed the set_default_kernel branch from 6bc10bc to a979167 Compare March 4, 2025 19:28

rh-jugraham mentioned this pull request Mar 4, 2025

in_place_upgrade_guest: automates the case that verifies in place upgrade on a guest autotest/tp-libvirt#6195

Merged

rh-jugraham force-pushed the set_default_kernel branch 4 times, most recently from 306951e to a47f936 Compare March 4, 2025 21:39

dzhengfy reviewed Mar 17, 2025

View reviewed changes

rh-jugraham force-pushed the set_default_kernel branch 3 times, most recently from e9e83b9 to cbc071b Compare March 19, 2025 22:05

rh-jugraham mentioned this pull request Mar 19, 2025

utils_sys: moved update_boot_option from utils_test #4087

Closed

dzhengfy approved these changes Mar 20, 2025

View reviewed changes

dzhengfy requested a review from smitterl March 20, 2025 07:09

rh-jugraham changed the title ~~utils_test: created function to update default kernel~~ utils_sys: created function to update default kernel Mar 20, 2025

rh-jugraham force-pushed the set_default_kernel branch 2 times, most recently from 0cc40ee to bddb26c Compare March 20, 2025 17:44

rh-jugraham changed the title ~~utils_sys: created function to update default kernel~~ utils_test: created function to update default kernel Mar 20, 2025

rh-jugraham requested a review from dzhengfy March 20, 2025 17:46

dzhengfy approved these changes Mar 21, 2025

View reviewed changes

smitterl requested changes Mar 24, 2025

View reviewed changes

smitterl reviewed Mar 24, 2025

View reviewed changes

Comment thread virttest/utils_test/__init__.py Outdated

smitterl reviewed Mar 24, 2025

View reviewed changes

Comment thread virttest/utils_test/__init__.py Outdated

rh-jugraham force-pushed the set_default_kernel branch from 35db5e7 to 6574487 Compare March 25, 2025 16:48

rh-jugraham force-pushed the set_default_kernel branch 2 times, most recently from eb5a6cb to 9cf857b Compare March 25, 2025 16:50

rh-jugraham requested a review from smitterl March 25, 2025 16:50

rh-jugraham force-pushed the set_default_kernel branch 4 times, most recently from e2ed3e4 to ea9578e Compare March 29, 2025 00:48

utils_test: created functions to update default kernel as well as get…

db5887b

… all kernel paths given some pattern Signed-off-by: Julia Graham <jugraham@redhat.com>

rh-jugraham force-pushed the set_default_kernel branch from ea9578e to db5887b Compare March 29, 2025 00:50

smitterl approved these changes Apr 4, 2025

View reviewed changes

dzhengfy merged commit 567b3b3 into avocado-framework:master Apr 7, 2025
50 checks passed

Conversation

rh-jugraham commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rh-jugraham commented Mar 4, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzhengfy commented Mar 17, 2025

Uh oh!

smitterl commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rh-jugraham commented Mar 19, 2025

Uh oh!

dzhengfy commented Mar 20, 2025

Uh oh!

dzhengfy commented Mar 20, 2025

Uh oh!

dzhengfy left a comment

Choose a reason for hiding this comment

Uh oh!

rh-jugraham commented Mar 20, 2025

Uh oh!

dzhengfy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smitterl Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rh-jugraham commented Mar 25, 2025

Uh oh!

smitterl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rh-jugraham commented Mar 4, 2025 •

edited

Loading

smitterl commented Mar 17, 2025 •

edited

Loading

smitterl Mar 27, 2025 •

edited

Loading